fix: Ensure InstrumentationScope is properly propagated. #3175

calavera · 2025-09-24T00:11:12Z

Changes

With the current implementation, there is no way for an InstrumentationScope to export its version and attributes correctly.

The reason why this is happening is because when the InstrumentationScope is exported, the target key is always Some. This is because when the code creates the scope_map, the target key always has a value, either comming from the log record's target, or from the name of the instrumentation scope.

That means that when the code calls InstrumentationScope::from((&InstrumentationScope, Option<Cow<'static, str>>)), the option is always Some. This causes the new InstrumentationScope to always from version and attributes from the incoming scope.

This change assumes that if the target and the instrumentation scope name match, the user wants to preserve the version and attributes of the incoming scope.

There are other alternatives implementations that check whether the log record target is None that could also be valid, but I think both would have the same outcome.

Please provide a brief description of the changes here.

Merge requirement checklist

CONTRIBUTING guidelines followed
Unit tests added/updated (if applicable)
Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
Changes in public API reviewed (if applicable)

codecov · 2025-09-24T00:13:59Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.7%. Comparing base (00f97c9) to head (827bad7).

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #3175   +/-   ##
=====================================
  Coverage   80.6%   80.7%           
=====================================
  Files        126     126           
  Lines      22331   22424   +93     
=====================================
+ Hits       18019   18112   +93     
  Misses      4312    4312

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

With the current implementation, there is no way for an InstrumentationScope to export its version and attributes correctly. The reason why this is happening is because when the InstrumentationScope is exported, the target key is always `Some`. This is because when the code creates the `scope_map`, the target key always has a value, either comming from the log record's target, or from the name of the instrumentation scope. That means that when the code calls `InstrumentationScope::from((&InstrumentationScope, Option<Cow<'static, str>>))`, the option is always `Some`. This causes the new InstrumentationScope to always from version and attributes from the incoming scope. This change assumes that if the target and the instrumentation scope name match, the user wants to preserve the version and attributes of the incoming scope. There are other alternatives implementations that check whether the log record target is None that could also be valid, but I think both would have the same outcome.

Copilot

Pull Request Overview

This PR fixes an issue where InstrumentationScope version and attributes were not being properly propagated during log export. The problem was that the target key in the scope_map always had a value, causing the InstrumentationScope to lose its original metadata when exported.

Extracts scope creation logic into a dedicated new_instrumentation_scope function with conditional logic
Preserves original scope attributes and version when the target matches the scope name
Adds comprehensive test coverage for different scenarios of scope attribute preservation

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-02T18:02:26Z

opentelemetry-proto/src/transform/logs.rs

+        let logs = [(&log_record1, &instrum_lib1), (&log_record2, &instrum_lib2)];
+        let log_batch = LogBatch::new(&logs);
+        let resource: ResourceAttributesWithSchema = (&resource).into(); // Convert Resource to ResourceAttributesWithSchema
+        let grouped_logs =
+            crate::transform::logs::tonic::group_logs_by_resource_and_scope(log_batch, &resource);
+
+        // This makes the grouping to match the existent InstrumentationScope, preserving the scope attributes.
+        log_record1.set_target("lib1");
+        log_record2.set_target("lib2");
+


The log record targets are being set after the log batch is created and grouped. This means the test is not actually testing the scenario described in the comment. Move these lines before creating the log batch to test the intended behavior.

Suggested change

let logs = [(&log_record1, &instrum_lib1), (&log_record2, &instrum_lib2)];

let log_batch = LogBatch::new(&logs);

let resource: ResourceAttributesWithSchema = (&resource).into(); // Convert Resource to ResourceAttributesWithSchema

let grouped_logs =

crate::transform::logs::tonic::group_logs_by_resource_and_scope(log_batch, &resource);

// This makes the grouping to match the existent InstrumentationScope, preserving the scope attributes.

log_record1.set_target("lib1");

log_record2.set_target("lib2");

// This makes the grouping to match the existent InstrumentationScope, preserving the scope attributes.

log_record1.set_target("lib1");

log_record2.set_target("lib2");

let logs = [(&log_record1, &instrum_lib1), (&log_record2, &instrum_lib2)];

let log_batch = LogBatch::new(&logs);

let resource: ResourceAttributesWithSchema = (&resource).into(); // Convert Resource to ResourceAttributesWithSchema

let grouped_logs =

crate::transform::logs::tonic::group_logs_by_resource_and_scope(log_batch, &resource);

Might be missing something, but I think copilot is right on this one - its not clear to me why the target is set after the call to group_logs_by_resource_and_scope

opentelemetry-proto/src/transform/logs.rs

scottgerring

Hey @calavera thanks for opening this, and sorry for the slow turn-around here!
Some comments inline; lmk what you think.

scottgerring · 2025-10-09T10:39:33Z

opentelemetry-proto/src/transform/logs.rs

+    ///
+    /// 1. If the log scope name matches the key, return a new scope with all the fields from the original scope.
+    /// 2. Otherwise, return a new scope with the key as the name of the scope, and the rest of the fields empty.
+    fn new_instrumentation_scope(


Did you consider pushing this down into InstrumentationScope::from? I've not dug into it in depth but it looks like the only place it is being used is the code you're touching, and i'm wondering if all in cases, the callers are going to expect this behaviour anyway.

I also think it would be great to document, somewhere, exactly how target relates to instrumentation scope. It took me a while to grok what's happening here:

This could be on the fn that does applies the relevant logic. Perhaps something like (overly detailed description to make sure i've not missed something 😄 ):

When you explicitly specify target in a log (e.g., error!(target: "security.audit", ...)), it overrides the default module path and becomes the InstrumentationScope.name
when the log is exported to OpenTelemetry backends.

If the target differs from the InstrumentationScope name, the exported scope will contain only that target
string as the name, with empty version and attributes.

This allows you to logically group logs under custom identifiers (like "postgres", "authentication", etc.) rather
than module paths, but at the cost of losing the instrumentation library's metadata.

opentelemetry-proto/src/transform/logs.rs

scottgerring · 2025-10-09T10:50:25Z

opentelemetry-proto/src/transform/logs.rs

+        let logs = [(&log_record1, &instrum_lib1), (&log_record2, &instrum_lib2)];
+        let log_batch = LogBatch::new(&logs);
+        let resource: ResourceAttributesWithSchema = (&resource).into(); // Convert Resource to ResourceAttributesWithSchema
+        let grouped_logs =
+            crate::transform::logs::tonic::group_logs_by_resource_and_scope(log_batch, &resource);
+
+        // This makes the grouping to match the existent InstrumentationScope, preserving the scope attributes.
+        log_record1.set_target("lib1");
+        log_record2.set_target("lib2");
+


Might be missing something, but I think copilot is right on this one - its not clear to me why the target is set after the call to group_logs_by_resource_and_scope

calavera requested a review from a team as a code owner September 24, 2025 00:11

calavera force-pushed the propagate_instrumentation_scope branch from 5e7f8da to ebe6e6e Compare September 24, 2025 15:53

calavera changed the title ~~Ensure InstrumentationScope is properly propagated.~~ fix: Ensure InstrumentationScope is properly propagated. Sep 24, 2025

calavera force-pushed the propagate_instrumentation_scope branch from ebe6e6e to c5cd7a9 Compare September 24, 2025 16:29

Merge branch 'main' into propagate_instrumentation_scope

827bad7

lalitb requested a review from Copilot October 2, 2025 18:01

Copilot AI reviewed Oct 2, 2025

View reviewed changes

scottgerring reviewed Oct 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Ensure InstrumentationScope is properly propagated. #3175

fix: Ensure InstrumentationScope is properly propagated. #3175

Uh oh!

calavera commented Sep 24, 2025

Uh oh!

codecov bot commented Sep 24, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Oct 2, 2025

Uh oh!

scottgerring Oct 9, 2025

Uh oh!

Uh oh!

scottgerring left a comment

Uh oh!

scottgerring Oct 9, 2025

Uh oh!

scottgerring Oct 9, 2025

Uh oh!

Uh oh!

scottgerring Oct 9, 2025

Uh oh!

Uh oh!

fix: Ensure InstrumentationScope is properly propagated. #3175

Are you sure you want to change the base?

fix: Ensure InstrumentationScope is properly propagated. #3175

Uh oh!

Conversation

calavera commented Sep 24, 2025

Changes

Merge requirement checklist

Uh oh!

codecov bot commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

scottgerring Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scottgerring left a comment

Choose a reason for hiding this comment

Uh oh!

scottgerring Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

scottgerring Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scottgerring Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Sep 24, 2025 •

edited

Loading